-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add codeListEditor in config options #13953
base: main
Are you sure you want to change the base?
Conversation
Still needs the following: - Tests for onClose in CodeListTableEditor.tsx - Check typing and tests overall.
…istEditorRow.tsx should likely be reverted. To be determined during review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 🎉
Some small questions and comments in general, and also a suggestion to adapt renderWithProviders
to make the tests more readable 🤗
...editor/src/components/config/editModal/EditOptions/EditCodeList/CodeListTableEditor.test.tsx
Outdated
Show resolved
Hide resolved
.../ux-editor/src/components/config/editModal/EditOptions/EditCodeList/utils/conversionUtils.ts
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/components/config/editModal/EditOptions/EditOptions.test.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/ux-editor/src/hooks/mutations/useUpdateOptionListMutation.ts
Outdated
Show resolved
Hide resolved
...s/ux-editor/src/components/config/editModal/EditOptions/EditCodeList/CodeListTableEditor.tsx
Outdated
Show resolved
Hide resolved
...s/ux-editor/src/components/config/editModal/EditOptions/EditCodeList/CodeListTableEditor.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13953 +/- ##
==========================================
- Coverage 95.29% 95.25% -0.04%
==========================================
Files 1653 1656 +3
Lines 22024 22081 +57
Branches 2589 2594 +5
==========================================
+ Hits 20988 21034 +46
- Misses 790 798 +8
- Partials 246 249 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better ⭐ I still have an suggestion to avoid the useEffect and undefined checks in CodeListEditor.tsx
🙈
import { useOptionListsQuery } from '../../../../../hooks/queries/useOptionListsQuery'; | ||
import { useCodeListEditorTexts } from './hooks/useCodeListEditorTexts'; | ||
import { usePreviewContext } from 'app-development/contexts/PreviewContext'; | ||
import { ErrorMessage } from '@digdir/designsystemet-react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a StudioErrorMessage now 🤗
setCodeList(options); | ||
}; | ||
|
||
if (component.optionsId === undefined) return <StudioSpinner spinnerTitle={'test'} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess this spinnerTitle should not be test
🙈
And do we need it? Why should there be a indefinite spinner when there is no optionsId connected to the component? As it is okey for a component not to have a connected optionList
if (component.optionsId === undefined) return <StudioSpinner spinnerTitle={'test'} />; | ||
switch (status) { | ||
case 'pending': | ||
return <StudioSpinner spinnerTitle={'test'} />; // Extract title to nb.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above 🙈 I see you have reminded yourself as well
case 'error': | ||
return ( | ||
<ErrorMessage> | ||
{error instanceof Error ? error.message : t('ux_editor.modal_properties_error_message')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we ever expose the backend error to the user? 🤔 I think it is okay to display a generic error in this iteration, and then we can potentially implement a better error handling in another issue that uses text-keys for errors received from api 🤗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments here:
- We only need to pass queries and the queryclient if we are modifying them. They will be defined in the renderWithProviders if they are not passed 🤗 As this test is now it does not look like it is modifying the queryClients cache so we dont need to pass it.
- I see that you await for the spinner to be removed. I see two different approaches here;
2.1: we can set the optionList directly on the cache if we anyway always will wait for the spinner to removed before executing the tests.
2.2: we can actually test that the spinner is there when it suppose to be. There are also two ways to implement that. Either you can set the cache in the renderMethod if data is passed to the renderMethod. Or you can use adapt the queries and have the waitForSpinnerTobeRemoved in a separete renderMethod. We have examples of both approcahes around the codebase. Let me know if you need help to find it 🤗
previewContextProps: previewContextProps, | ||
}, | ||
); | ||
await waitForElementToBeRemoved(screen.queryByTestId('studio-spinner-test-id')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you decide to keep the waitForElementToBeRemoved method we should use the text on it instead of test Id I think 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey, I have a suggestion to simplify this code by separating a bit more. It will be too difficult to describe my suggestion so I will simply paste an updated version of the file:
import React, { createRef, useState } from 'react';
import type { IGenericEditComponent } from '@altinn/ux-editor/components/config/componentConfig';
import type { SelectionComponentType } from '@altinn/ux-editor/types/FormComponent';
import type { Option } from 'app-shared/types/Option';
import type { ApiError } from 'app-shared/types/api/ApiError';
import type { AxiosError } from 'axios';
import { useTranslation } from 'react-i18next';
import {
StudioCodeListEditor,
StudioModal,
StudioSpinner,
type CodeList,
} from '@studio/components';
import { TableIcon } from '@studio/icons';
import { useStudioEnvironmentParams } from 'app-shared/hooks/useStudioEnvironmentParams';
import { useUpdateOptionListMutation } from '../../../../../hooks/mutations/useUpdateOptionListMutation';
import { useOptionListsQuery } from '../../../../../hooks/queries/useOptionListsQuery';
import { useCodeListEditorTexts } from './hooks/useCodeListEditorTexts';
import { usePreviewContext } from 'app-development/contexts/PreviewContext';
import { ErrorMessage } from '@digdir/designsystemet-react';
import classes from './CodeListEditor.module.css';
type CodeListEditorProps = Pick<IGenericEditComponent<SelectionComponentType>, 'component'>;
export function CodeListEditor({ component }: CodeListEditorProps): React.ReactNode {
const { t } = useTranslation();
const { org, app } = useStudioEnvironmentParams();
const { data: optionsListMap, status, error } = useOptionListsQuery(org, app);
switch (status) {
case 'pending':
return <StudioSpinner spinnerTitle={'test'} />; // Extract title to nb.json
case 'error':
return (
<ErrorMessage>
{t('ux_editor.modal_properties_error_message')}
</ErrorMessage>
);
case 'success': {
return (
<CodeListEditorModal
codeList={optionsListMap[component.optionsId]}
component={component}
/>
);
}
}
}
type CodeListEditorModalProps = {
codeList: CodeList;
} & Pick<IGenericEditComponent<SelectionComponentType>, 'component'>;
function CodeListEditorModal({
codeList,
component,
}: CodeListEditorModalProps): React.ReactNode {
const { t } = useTranslation();
const { org, app } = useStudioEnvironmentParams();
const { doReloadPreview } = usePreviewContext();
const { mutate: uploadOptionList } = useUpdateOptionListMutation(org, app, {
hideDefaultError: (apiError: AxiosError<ApiError>) => !apiError.response.data.errorCode,
});
const editorTexts = useCodeListEditorTexts();
const modalRef = createRef<HTMLDialogElement>();
const [currentCodeList, setCodeList] = useState<CodeList>(codeList);
const handleOptionsChange = (options?: Option[]) => {
setCodeList(options);
};
const handleClose = () => {
uploadOptionList({ optionListId: component.optionsId, optionsList: currentCodeList });
doReloadPreview();
modalRef.current?.close();
};
return (
<StudioModal.Root>
<StudioModal.Trigger
className={classes.modalTrigger}
variant='secondary'
icon={<TableIcon />}
>
{t('ux_editor.modal_properties_code_list_open_editor')}
</StudioModal.Trigger>
<StudioModal.Dialog
ref={modalRef}
className={classes.manualTabModal}
closeButtonTitle={t('general.close')}
heading={t('ux_editor.modal_add_options_codelist')}
onBeforeClose={handleClose}
onInteractOutside={handleClose}
>
<StudioCodeListEditor
codeList={currentCodeList}
onChange={handleOptionsChange}
texts={editorTexts}
/>
</StudioModal.Dialog>
</StudioModal.Root>
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a test here that checks if the feature is available if the feature is enabled? 🧐
renderOptions={renderOptions} | ||
/>, | ||
{ | ||
queries, | ||
queries: queries, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont think this is necessary 🙈
And I forgot to comment that maybe we should use the term optionList all places here? As we discussed in the meeting earlier 😬 |
<StudioModal.Root> | ||
<StudioModal.Trigger | ||
className={classes.modalTrigger} | ||
variant='secondary' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And should this button be variant='tertiary'
? 🤔
Description
Added
StudioCodeListEditor
component to options config, through new component namedCodeListTableEditor
.The component is hidden behind the featureFlag
codeListEditor
, as we do not wish to change prod before studio supports multiple types, and merging the view for choosing or editing codelists into 1 tab.Video
Skjermopptak.2024-10-30.142753.mp4
Out of scope:
Related Issue(s)
Verification
Documentation